refactor(shared-data, robot-server, api): Pipette configuration architecture refactor to organize by nozzle map and tip type #15250
Conversation
…generic run with 96 channel
…ration identification
| def _get_matching_approved_nozzle_map(self) -> str: | ||
| for map_key in self._valid_nozzle_maps.maps.keys(): | ||
| if self._valid_nozzle_maps.maps[map_key] == list( | ||
| self._nozzle_manager.current_configuration.map_store.keys() |
There was a problem hiding this comment.
this is a whole lot to be doing just here, and it adds this error category. instead of doing this, let's tell the nozzle manager what map key it's using and then we can look up the configuration based on that directly.
| self._working_volume = min(tip_type.value, self.liquid_class.max_volume) | ||
|
|
||
| def _get_matching_approved_nozzle_map(self) -> str: | ||
| for map_key in self._valid_nozzle_maps.maps.keys(): |
| approved_map = self._get_matching_approved_nozzle_map() | ||
| try: | ||
| if isinstance(config, PressFitPickUpTipConfiguration) and all( | ||
| [ |
There was a problem hiding this comment.
maybe simplify this by putting config.configuration_by_nozzle_map[approved_map] in an intermediate
| ): | ||
| approved_map = map_key | ||
| if approved_map is None: | ||
| raise ValueError( |
There was a problem hiding this comment.
should be a protocol engine error that descends from an enumerated error
| return config.configuration_by_nozzle_map[ | ||
| self._nozzle_manager.current_configuration.valid_map_key | ||
| ][pip_types.PipetteTipType(self._liquid_class.max_volume).name].speed | ||
| except KeyError: |
There was a problem hiding this comment.
we should have these errors be enumerated errors defined in shared data using an error code, like a new 4000-series "missing configuration data" or something
| self._nozzle_manager.current_configuration.valid_map_key | ||
| ][pip_types.PipetteTipType(self._liquid_class.max_volume).name].speed | ||
| except KeyError: | ||
| default = config.configuration_by_nozzle_map[ |
There was a problem hiding this comment.
Could this be a free function in a shared area? I think it's an identical implementation for the two machines
| except KeyError: | ||
| default = config.configuration_by_nozzle_map[ | ||
| self._nozzle_manager.current_configuration.valid_map_key | ||
| ].get("default") |
There was a problem hiding this comment.
and actually generally the task of "get data from the unified structure" might be able to be commonized further
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #15250 +/- ##
==========================================
- Coverage 63.20% 63.18% -0.02%
==========================================
Files 287 287
Lines 14891 14875 -16
==========================================
- Hits 9412 9399 -13
+ Misses 5479 5476 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
CaseyBatten
requested review from
b-cooper and
nusrat813
and removed request for
a team
|
|
||
|
|
||
| class MissingConfigurationData(GeneralError): | ||
| """An error indicating that provided configurationd ata is missing or invalid. |
There was a problem hiding this comment.
| """An error indicating that provided configurationd ata is missing or invalid. | |
| """An error indicating that provided configuration data is missing or invalid. |
There was a problem hiding this comment.
Looks like it will work, and good job - this was pretty hairy! Would love to see the virtual static pipette checks raise enumerated errors and I think there's a solid followup that takes some of these functions that have like 3 layers of try/except logic and factors them into using some intermediate accessors, though.
| @@ -280,9 +284,20 @@ def build( | |||
| f"Partial Nozzle Layouts may not be configured to contain more than {MAXIMUM_NOZZLE_COUNT} channels." | |||
| ) | |||
|
|
|||
| validated_map_key = None | |||
| for map_key in valid_nozzle_maps.maps.keys(): | |||
| if valid_nozzle_maps.maps[map_key] == list(map_store.keys()): | |||
There was a problem hiding this comment.
Validated maps are ordered from the smallest validated configuration to the largest in our definitions, with nozzle sets in each map following a Column then Row mapping order, the same as our map store.
| ): | ||
| approved_map = map_key | ||
| if approved_map is None: | ||
| raise ValueError( |
Overview
Covers PLAT-334, with effects on PLAT-270, PLAT-269
Addresses concerns with the capability of pipette definitions to reflect the reality of pipette hardware performance and necessary configurations for any given nozzle layout/tip type combination.
A follow up PR will be expected to utilize the schema structure introduced here to implement new values for various pipettes and configurations.
Test Plan
Testing plan will involve ensuring migration of pipette definitions had maintained expected performance (speed of operation, pick up tip success) from previous infrastructure.
In terms of capability, the following protocols should pass analysis and execute successfully on a Flex and OT2.
Changelog
Review requests
Please take a look at the pipette definitions schema and the migrated definitions for the pipettes. Does this seem appropriate and comprehensive enough to capture the data we are seeking to include in our system? Does the engine side implementation of this new approach seem appropriate?
Risk assessment
Medium - While most of these changes are to the pipette schema and non-invasive to engine logic, I would like a careful eye/testing regiment for the various pipette definitions that have been migrated to the new schema type to ensure performance remains the same without regressions.